Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GitHub alert block quotes #118

Merged

Conversation

tuurep
Copy link
Collaborator

@tuurep tuurep commented Jul 22, 2024

Close #89

@tuurep tuurep changed the title Issue/89 GitHub alert block quotes GitHub alert block quotes Jul 22, 2024
@Tweekism
Copy link
Collaborator

Lookin goooood, I'm excited 😋

@tuurep
Copy link
Collaborator Author

tuurep commented Jul 22, 2024

Ok @Tweekism @jannis-baum this is ready for feedback

I basically took what's here:

https://github.com/antfu/markdown-it-github-alerts/blob/main/styles/github-base.css

But fit it into our style.css, maybe removing some redundant lines

Screenshots:

ghalert1
ghalert2

My concerns:

  • Would there be a way to plug the styles from the plugin without essentially copy pasting CSS? Are they on the repo just for reference or supposed to be able to be used directly?
  • What do you think about those ANSI colors on dark? To me looks a bit hit and miss.. Some are ok, but some too light, notably yellow.
    • They are kinda ok though :)

@tuurep tuurep marked this pull request as ready for review July 22, 2024 19:38
@tuurep tuurep force-pushed the issue/89-github-alert-block-quotes branch from 6590858 to 25bbe2c Compare July 22, 2024 19:48
@jannis-baum
Copy link
Owner

Hey thanks! I will look into the code later or tomorrow :)

Would there be a way to plug the styles from the plugin without essentially copy pasting CSS? Are they on the repo just for reference or supposed to be able to be used directly?

Not sure. On giving their docs one short look I couldn't find instructions. Might also be that our set up is a bit too exotic for this to work. One cheeky thing you could try however is making a symlink in the static directory to their styles at node_modules/....css and then adding that symlink to be included in the as styles in our viewer.ts ^^ Might just work. If it works on dev but not for the build we can fix it. If it doesn't work on dev it would get a bit ugly I think

@Tweekism
Copy link
Collaborator

  • What do you think about those ANSI colors on dark? To me looks a bit hit and miss.. Some are ok, but some too light, notably yellow.

    • They are kinda ok though :)

These colors are workable, I missed the context for why ANSI colors. Is the intention to use the same colors in the terminal in vim?

@jannis-baum
Copy link
Owner

These colors are workable, I missed the context for why ANSI colors. Is the intention to use the same colors in the terminal in vim?

From #89 (comment) onwards. We were thinking to reuse ANSI-colors because we also need them for rendering Jupyter Notebooks.

What do you think about those ANSI colors on dark? To me looks a bit hit and miss.. Some are ok, but some too light, notably yellow.

I honestly don't like them that much to be honest🙈

@tuurep
Copy link
Collaborator Author

tuurep commented Jul 23, 2024

What do you think about those ANSI colors on dark? To me looks a bit hit and miss.. Some are ok, but some too light, notably yellow.

I honestly don't like them that much to be honest🙈

That's a good answer 😄 What kind would work best?

My thought is that they could be like the light theme colors but adapted to dark, then adjust and whatever

Wdyt about that or did you have something else in mind?

I can make an initial suggestion if needed

@jannis-baum
Copy link
Owner

That's a good answer 😄 What kind would work best?

My thought is that they could be like the light theme colors but adapted to dark, then adjust and whatever

Wdyt about that or did you have something else in mind?

I can make an initial suggestion if needed

I honestly don't have any idea ^^ Would be glad about a suggestion! I did see however that the plugin also has a dark theme CSS (https://github.com/antfu/markdown-it-github-alerts/tree/main/styles) so if you want to go the route of adjusting the light colors to dark that might already be done there :)

@tuurep
Copy link
Collaborator Author

tuurep commented Jul 23, 2024

One cheeky thing you could try however is making a symlink in the static directory to their styles at node_modules/....css and then adding that symlink to be included in the as styles in our viewer.ts ^^ Might just work.

Seems like such hackery 😄 Maybe it's worth it to have all the CSS in there with the rest and avoid this?

But I'm kinda interested in what the function of the CSS in the repo is. Whether that's some markdown-it plugin convention and there's more info about that. I'll see.

@tuurep
Copy link
Collaborator Author

tuurep commented Jul 23, 2024

I did see however that the plugin also has a dark theme CSS (https://github.com/antfu/markdown-it-github-alerts/tree/main/styles) so if you want to go the route of adjusting the light colors to dark that might already be done there :)

Yeah I can try this as an initial step

The dark theme is "GitHub Dark" and notable that it has quite a different background color than yours

I could also look at GitHub Dark High Contrast alert colors

@jannis-baum
Copy link
Owner

jannis-baum commented Jul 23, 2024

But I'm kinda interested in what the function of the CSS in the repo is. Whether that's some markdown-it plugin convention and there's more info about that. I'll see.

I think it's some bigger thing than just markdown-it; I have definitely seen similar ways of importing CSS in NextJS and React so there is probably some big widely used library that does this. I don't know how applicable it is to our use-case of serving the file statically with express though. If we want to do that we might have to change quite a bit of stuff and start using JSX with some other dependency. But yes, maybe it's less bad; might be worth investigating!

@jannis-baum
Copy link
Owner

The dark theme is "GitHub Dark" and notable that it has quite a different background color than yours

Good point!

I could also look at GitHub Dark High Contrast alert colors

Yes true. Might just be that those colors "pop" a bit too much but maybe it's a good starting point anyways.

@tuurep
Copy link
Collaborator Author

tuurep commented Jul 23, 2024

⚠️ I'll go deep into GitHub colors

The markdown-it plugin repo CSS has slight differences in colors to what they actually are on GitHub as of this moment, but it does look like they were taken from the GitHub variables directly, ~8 months ago:

antfu/markdown-it-github-alerts@595f578

(due to the same variable names that you see when you inspect GH)

Speculation: I guess that GH adjusts these colors over time, and CSS such like in the plugin doesn't keep up

So I would take the colors straight from inspecting GH. But lets look at screenshots for:

  1. Plugin CSS
  2. Straight from GitHub CSS
    • For dark: GH Dark and GH Dark High Contrast

Light

  • Only the red and yellow (warning/caution) are different
    • (though for the red you'll need a magnifying glass)

Plugin repo CSS

image

GitHub up-to-date CSS

image

Dark

  • All colors are different

Plugin repo CSS

image

GitHub up-to-date Dark CSS

image

GitHub up-to-date Dark High Contrast CSS

image

GitHub up-to-date Dark Dimmed CSS (basically Low Contrast)

image

@tuurep
Copy link
Collaborator Author

tuurep commented Jul 23, 2024

So in my opinion any of the last 3 (note: I added a "low contrast") could work for dark, and all look pretty good

I would avoid the outdated plugin CSS

Thoughts @jannis-baum ?

@tuurep
Copy link
Collaborator Author

tuurep commented Jul 23, 2024

Default:

#1f6feb
#238636
#8957e5
#9e6a03
#da3633

High contrast:

#409eff
#09b43a
#b87fff
#e09b13
#ff6a69

Dimmed:

#316dca
#347d39
#8256d0
#966600
#c93c37

@Tweekism
Copy link
Collaborator

Ok yeah the up to date dimmed does look a lot nicer.

@tuurep
Copy link
Collaborator Author

tuurep commented Jul 23, 2024

Yeah it could be a fit!

To be perfectly clear the Dimmed one is not something I created, but it's a theme on GitHub:

image

@tuurep tuurep mentioned this pull request Jul 23, 2024
3 tasks
@jannis-baum
Copy link
Owner

Hey, sorry I've been taking so long to reply to this, I've been back and forth on the colors because I am not that happy with any of them. They all seem a bit dull to me and the warning color is very brown.

I just checked through Jellyfish again and realized I have colors for compiler notes, warnings and errors that might be quite suitable. So if we use those three plus the ANSI colors for "tip" and "important" it would look like this:

image
:root {
    --color-alert-note: #afafff;
    --color-alert-tip: #b4fa72;
    --color-alert-important: #ff8ffd;
    --color-alert-warning: #ffaf00;
    --color-alert-caution: #ff5f5f;
}

What do you think?

@Tweekism
Copy link
Collaborator

I'm a little triggered that Note has shifted from blue to more of a purple 😂

And it might be a little too similar to Important?

@tuurep
Copy link
Collaborator Author

tuurep commented Jul 25, 2024

I was just about to comment too

It's definitely an improvement from the first

(this)
alert1

I think it's pretty fine tbh and I'd approve, but if we need to adjust then lets think about it

Safe to say lets not use the GH colors directly, if they look out of place in the theme

@jannis-baum
Copy link
Owner

Soooo how about falling back to the ANSI color for Note as well?

image

:root {
    --color-alert-note: #a5d5fe;
    --color-alert-tip: #b4fa72;
    --color-alert-important: #ff8ffd;
    --color-alert-warning: #ffaf00;
    --color-alert-caution: #ff5f5f;
}

@Tweekism
Copy link
Collaborator

I like it, I like that our pop a little more. I'd be happy with either honestly.

I know no one was asking, but here is an up close comparison with the git hub ones.

image

@Tweekism
Copy link
Collaborator

Its easy to tweak a single colour in custom css, but it is important to have sensible defaults

@tuurep
Copy link
Collaborator Author

tuurep commented Jul 25, 2024

it is important to have sensible defaults

Yeah I agree with this very much :)

I have a little feeling that the blue is just a bit too light, similarly to the ANSI yellow but not nearly as severe.

But the more I've stared it and tried to come up with an improvement, the more I think it's kinda OK enough.

@jannis-baum
Copy link
Owner

Hahaha okay so let's just merge this? In case we end up disliking it too much we can tweak it when we refactor the CSS in #120. That one will probably be a huge color discussion anyways😂

@jannis-baum
Copy link
Owner

jannis-baum commented Jul 25, 2024

Oh but before we merge, @tuurep do you want to commit the latest version we agreed on now? I.e. ANSI plus the warning and error colors.

:root {
    --color-alert-note: #a5d5fe;
    --color-alert-tip: #b4fa72;
    --color-alert-important: #ff8ffd;
    --color-alert-warning: #ffaf00;
    --color-alert-caution: #ff5f5f;
}

@tuurep
Copy link
Collaborator Author

tuurep commented Jul 25, 2024

Alright, done!

@jannis-baum jannis-baum merged commit c4aceaf into jannis-baum:main Jul 25, 2024
5 checks passed
@tuurep tuurep deleted the issue/89-github-alert-block-quotes branch July 25, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitHub alert block quotes
3 participants